Skip to content

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Jan 20, 2025

Description

  • Query (C)UDNs using K8S API
  • Display empty (C)UDNs in the topology view

image

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
    • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
    • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
    • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
    • Standard QE validation, with pre-merge tests unless stated otherwise.
    • Regression tests only (e.g. refactoring with no user-facing change).
    • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

@openshift-ci
Copy link

openshift-ci bot commented Jan 20, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@jpinsonneau
Copy link
Contributor Author

jpinsonneau commented Jan 20, 2025

@jotak & @msherif1234 any thoughts about the approach ?
I still need to find a way to highlight empty UDNs in the topology view.

That approach could be useful for other scenarios such as Nodes / Namespaces especially since we are introducing capture filters. The UI could show we don't capture everything in these cases

v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/dynamic"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relying on dynamic client here allows us to read the CRDs without importing the whole API.

Comment on lines 22 to 42
cudns, err := resources.List(ctx, token, schema.GroupVersionResource{
Group: "k8s.ovn.org",
Version: "v1",
Resource: "clusteruserdefinednetworks",
})
if err != nil {
writeError(w, http.StatusInternalServerError, err.Error())
}

udns, err := resources.List(ctx, token, schema.GroupVersionResource{
Group: "k8s.ovn.org",
Version: "v1",
Resource: "userdefinednetworks",
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need to maintain the version per group / resource

@codecov
Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 34.61538% with 17 lines in your changes missing coverage. Please review.

Project coverage is 58.99%. Comparing base (7fd6c46) to head (f5f2edd).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
web/src/components/netflow-traffic.tsx 0.00% 8 Missing ⚠️
web/src/model/topology.ts 0.00% 6 Missing ⚠️
...omponents/drawer/element/element-panel-content.tsx 80.00% 1 Missing ⚠️
...ents/tabs/netflow-topology/2d/styles/styleNode.tsx 0.00% 1 Missing ⚠️
...ponents/tabs/netflow-topology/netflow-topology.tsx 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #688      +/-   ##
==========================================
+ Coverage   56.62%   58.99%   +2.36%     
==========================================
  Files         197      159      -38     
  Lines       10210     6974    -3236     
  Branches     1196     1199       +3     
==========================================
- Hits         5781     4114    -1667     
+ Misses       4059     2658    -1401     
+ Partials      370      202     -168     
Flag Coverage Δ
uitests 58.99% <34.61%> (-0.10%) ⬇️
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...eb/src/components/drawer/element/element-panel.tsx 73.33% <100.00%> (+0.91%) ⬆️
...b/src/components/drawer/netflow-traffic-drawer.tsx 45.54% <ø> (ø)
...ents/tabs/netflow-topology/2d/topology-content.tsx 29.23% <ø> (ø)
web/src/model/netflow-traffic.ts 86.90% <100.00%> (+0.15%) ⬆️
...omponents/drawer/element/element-panel-content.tsx 67.34% <80.00%> (+1.43%) ⬆️
...ents/tabs/netflow-topology/2d/styles/styleNode.tsx 29.16% <0.00%> (-0.63%) ⬇️
...ponents/tabs/netflow-topology/netflow-topology.tsx 36.26% <66.66%> (+1.03%) ⬆️
web/src/model/topology.ts 25.83% <0.00%> (-0.77%) ⬇️
web/src/components/netflow-traffic.tsx 65.25% <0.00%> (-1.51%) ⬇️

... and 38 files with indirect coverage changes

values := []string{}
for _, cudn := range cudns {
md := cudn.Object["metadata"].(map[string]interface{})
values = append(values, fmt.Sprintf("%s", md["name"]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
values = append(values, fmt.Sprintf("%s", md["name"]))
values = append(values, md["name"])

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I commited this but I will revert it as the metadata can contains other things than strings such as boolean

Resource: "clusteruserdefinednetworks",
})
if err != nil {
writeError(w, http.StatusInternalServerError, err.Error())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make sense I think to propagate the k8s error code, such as 401/403 ?
I never tried that but I guess it should work:

	kerr "k8s.io/apimachinery/pkg/api/errors"
//...
		if err != nil {
			var k8sErr *kerr.StatusError
			if errors.As(err, &k8sErr) {
				writeError(w, int(k8sErr.ErrStatus.Code), err.Error())
			} else {
				writeError(w, http.StatusInternalServerError, err.Error())
			}
		}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 96a8ce4

Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Apr 7, 2025
@Amoghrd
Copy link
Member

Amoghrd commented Apr 25, 2025

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Apr 25, 2025
@github-actions
Copy link

New image:
quay.io/netobserv/network-observability-console-plugin:9b2cd3a

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=9b2cd3a make set-plugin-image

@openshift-ci openshift-ci bot removed the lgtm label Apr 30, 2025
@openshift-ci
Copy link

openshift-ci bot commented Apr 30, 2025

New changes are detected. LGTM label has been removed.

@openshift-ci
Copy link

openshift-ci bot commented Apr 30, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jotak. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Apr 30, 2025
@jpinsonneau jpinsonneau added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Apr 30, 2025
@github-actions
Copy link

New image:
quay.io/netobserv/network-observability-console-plugin:6c19a18

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=6c19a18 make set-plugin-image

@jpinsonneau
Copy link
Contributor Author

Scope id changed from udn to network => f5f922b

@openshift-ci
Copy link

openshift-ci bot commented Apr 30, 2025

@jpinsonneau: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/plugin-cypress f5f922b link true /test plugin-cypress
ci/prow/qe-e2e-console-tests f5f922b link false /test qe-e2e-console-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@Amoghrd
Copy link
Member

Amoghrd commented Apr 30, 2025

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved QE has approved this pull request label Apr 30, 2025
@jpinsonneau jpinsonneau merged commit 2c3b33b into netobserv:main May 5, 2025
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. qe-approved QE has approved this pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants